Skip to content

Cleanup compliments module#2965

Merged
khassel merged 9 commits into
MagicMirrorOrg:developfrom
rejas:compliments
Dec 10, 2022
Merged

Cleanup compliments module#2965
khassel merged 9 commits into
MagicMirrorOrg:developfrom
rejas:compliments

Conversation

@rejas

@rejas rejas commented Nov 6, 2022

Copy link
Copy Markdown
Collaborator

Lots of small fixes and cleanups:

  • only render something when there is a compliment
  • cleanup naming
  • use es6 notations
  • use fetch instead of XMLHttpRequest in compliments

@rejas

rejas commented Nov 6, 2022

Copy link
Copy Markdown
Collaborator Author

@khassel asking you as the test-specialist ;-) : how can I fix the tests not finding "fetch" when I switch the module xmlhttprequest to fetch?

  ● Compliments module › remoteFile option › should show compliments from a remote file

    ReferenceError: fetch is not defined

@khassel

khassel commented Nov 6, 2022

Copy link
Copy Markdown
Collaborator

will try to look at this but I have very limited time until end of this month ...

@ghost

ghost commented Nov 7, 2022

Copy link
Copy Markdown

This was one of the simplest modules that worked properly, now is not working with HTML compliments like:

	"day_sunny": [
		"<i class=\"gold wi wi-day-sunny\"></i> Sunny day",
	],

@rejas

rejas commented Nov 7, 2022

Copy link
Copy Markdown
Collaborator Author

This was one of the simplest modules that worked properly, now is not working with HTML compliments like:

	"day_sunny": [
		"<i class=\"gold wi wi-day-sunny\"></i> Sunny day",
	],

when did it stop working? with this PR?

@ghost

ghost commented Nov 8, 2022

Copy link
Copy Markdown

This was one of the simplest modules that worked properly, now is not working with HTML compliments like:

	"day_sunny": [
		"<i class=\"gold wi wi-day-sunny\"></i> Sunny day",
	],

when did it stop working? with this PR?

yes

@sdetweil

sdetweil commented Nov 8, 2022

Copy link
Copy Markdown
Collaborator

not sure, but this moved the span inside the newline loop. so u will have a span per part.. instead of one.

and if split didn't, then you would lose the span

@rejas

rejas commented Nov 8, 2022

Copy link
Copy Markdown
Collaborator Author

This was one of the simplest modules that worked properly, now is not working with HTML compliments like:

	"day_sunny": [
		"<i class=\"gold wi wi-day-sunny\"></i> Sunny day",
	],

when did it stop working? with this PR?

yes

This doesnt work for me neithe ron the develop branch nor on the master branch. What version of MM2 are you using and how does your config look like in total (minus your secrets)?

@khassel

khassel commented Nov 8, 2022

Copy link
Copy Markdown
Collaborator

how can I fix the tests not finding "fetch" when I switch the module xmlhttprequest to fetch?

I don't know. This is the first time someone is using the browser fetch function.

The e2e tests are running with jsdom and it seems that fetch is not implemented there, see jsdom/jsdom#1724. There is a workaround, may you are able to get this working (but I'm not sure if it is worth to get another devDependency for one test).

@khassel

khassel commented Nov 8, 2022

Copy link
Copy Markdown
Collaborator

adding the line dom.window.fetch = corefetch; in the following function of tests/e2e/helpers/global-setup.js seems to work:

exports.getDocument = () => {
        return new Promise((resolve) => {
                const url = "http://" + (config.address || "localhost") + ":" + (config.port || "8080");
                jsdom.JSDOM.fromURL(url, { resources: "usable", runScripts: "dangerously" }).then((dom) => {
                        dom.window.name = "jsdom";
                        dom.window.fetch = corefetch;
                        dom.window.onload = () => {
                                global.document = dom.window.document;
                                resolve();
                        };
                });
        });
};

@codecov-commenter

codecov-commenter commented Nov 9, 2022

Copy link
Copy Markdown

Codecov Report

Merging #2965 (4a93f06) into develop (eee289a) will increase coverage by 0.09%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop    #2965      +/-   ##
===========================================
+ Coverage    22.75%   22.85%   +0.09%     
===========================================
  Files           51       51              
  Lines        10895    10887       -8     
===========================================
+ Hits          2479     2488       +9     
+ Misses        8416     8399      -17     
Impacted Files Coverage Δ
modules/default/compliments/compliments.js 0.00% <0.00%> (ø)
modules/default/updatenotification/node_helper.js 80.00% <0.00%> (+12.85%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@rejas rejas marked this pull request as ready for review November 9, 2022 09:41
@rejas

rejas commented Nov 9, 2022

Copy link
Copy Markdown
Collaborator Author

adding the line dom.window.fetch = corefetch; in the following function of tests/e2e/helpers/global-setup.js seems to work: ```

That did indeed help, thx!

@rejas rejas requested a review from khassel November 14, 2022 14:43

@khassel khassel left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this but I don't know if the other commenters are ...

@rejas

rejas commented Nov 23, 2022

Copy link
Copy Markdown
Collaborator Author

I'm fine with this but I don't know if the other commenters are ...

Cleaned up after @sdetweil's comment so that only one span is now generated again.

As for @Badatheist I cannot reproduce that. Doesnt work on develop nor on the last master release. Without more information I cannot do anything here.

@ghost

ghost commented Nov 26, 2022

Copy link
Copy Markdown

As for @Badatheist I cannot reproduce that. Doesnt work on develop nor on the last master release. Without more information I cannot do anything here.

What more information? It's very simple, before the compliments worked with HTML format, after the change they don't work anymore. I will stick to the old version which works perfectly for me. Not every update is good if it is not properly thought out and tested, it is valid for any software. For me it's a bug. Good luck and stay healthy!

@rejas

rejas commented Nov 26, 2022

Copy link
Copy Markdown
Collaborator Author

@Badatheist sorry to hear that, but like I wrote: I couldnt get your config entry to work, neither with my changes of course, but neither with the develop branch nor with the last release.

Without more information from your side (for example what version are you currently running, what is your whole config) I cannot help you out and get this bug fixed.

@khassel khassel merged commit a262444 into MagicMirrorOrg:develop Dec 10, 2022
@rejas rejas deleted the compliments branch December 10, 2022 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants